-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support new "generations" of existing segments #1
Conversation
…untouched partitions.
|
||
@SuppressWarnings("unchecked") | ||
private VersionType getSourceVersion(VersionType version) { | ||
if (version instanceof String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When could VersionType
be something other that String
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it is always String
in practice, but it would not be nice to add implicit type constraints to the previous functionality of this class.
VersionType sourceVersion = getSourceVersion(entry.getValue().getVersion()); | ||
TimelineEntry sourceEntry = sourceVersion != null ? versionEntry.get(sourceVersion) : null; | ||
|
||
if (sourceEntry != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some comments on what's happening here and why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Also added the main description of the PR to SegmentGenerationUtils
class documentation.
// For each timeline entry, add partitions from the source version that do not exist for the existing timeline | ||
// object. These are added as a separate entry to the list. | ||
|
||
for (int i = 0; i < initialSize; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the :
iteration here? It looks like i
is not used for any purpose other than holders.get(i)
, unless I fail to find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same list is being added to in this loop, only the original items should be iterated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this as a code comment, before someone will replace this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
segments | ||
); | ||
|
||
dbSegments.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be in a finally
block, or could you use try-with-resources
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using try-with-resources now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A segment with version containing
_gen_
is considered to be a new generation of an existing segment, and the version it is generated from (source version) matches the part before_gen_
.A generated segment has the following properties:
Additionally changed the default ordering of numbered partitions - the partition number now has a higher priority in comparisons than the core partition count. Before, if you had segments with
(partitionNumber, corePartitionCount)
of(0, 2), (1, 2), (2,0)
, this would not have been considered a complete set of partitions since they were sorted to and verified in the order(2,0), (0, 2), (1, 2)
.